Skip to content

feat: persist transport flags via apm config#1308

Open
Aaryan-Dadu wants to merge 6 commits into
microsoft:mainfrom
Aaryan-Dadu:feat/1243
Open

feat: persist transport flags via apm config#1308
Aaryan-Dadu wants to merge 6 commits into
microsoft:mainfrom
Aaryan-Dadu:feat/1243

Conversation

@Aaryan-Dadu
Copy link
Copy Markdown

Description

Users who authenticate to GitHub via SSH keys (no PAT configured) today
must pass --ssh or --allow-protocol-fallback on every apm install
invocation, or export environment variables in their shell profile which is a
cross-platform friction point (especially on Windows PowerShell / cmd).
This PR adds allow-protocol-fallback and ssh as first-class
apm config keys, so users can persist their transport preference once
and forget about it:

# Persist SSH preference for all future installs
apm config set ssh true
# Persist cross-protocol fallback
apm config set allow-protocol-fallback true

Changes:

  • config.py: add get/set_allow_protocol_fallback(), get/set_ssh(), get_apm_allow_protocol_fallback() and get_apm_protocol_pref() resolution helpers that encode the full env > config > default chain
  • commands/install.py: wire config into the transport-selection block;
  • commands/config.py: register both keys in setters/getters/valid-keys, add them to all listing paths (rich table, plain-text, get all-keys)
  • docs/reference/cli/config.md: new rows in keys table, resolution-order
    section for transport keys, usage examples
  • docs/reference/environment-variables.md: new Transport and protocol section documenting APM_GIT_PROTOCOL and APM_ALLOW_PROTOCOL_FALLBACK
  • tests: new test cases across test_config_command.py and new test_protocol_config_precedence.py

Closes #1243.

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Maintenance / refactor

Testing

  • Tested locally
  • All existing tests pass
  • Added tests for new functionality (if applicable)

Copilot AI review requested due to automatic review settings May 13, 2026 10:57
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds support for persisting transport-selection preferences via apm config, so users no longer need to re-specify --ssh / --allow-protocol-fallback (or set env vars) on every apm install. This extends the existing config surface and wires the effective-value resolution into the install transport-selection path.

Changes:

  • Introduces new persisted config keys: allow-protocol-fallback and ssh, plus helpers that resolve env > config > default.
  • Wires apm install transport selection to read the effective preference/fallback from config helpers when CLI flags are not provided.
  • Expands apm config CLI support (valid keys, set/get/list output) and adds docs + unit tests for precedence and CLI behavior.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/apm_cli/config.py Adds stored getters/setters for transport flags and env/config resolution helpers used by install.
src/apm_cli/commands/install.py Uses config helpers to resolve protocol preference and fallback behavior when CLI flags are absent.
src/apm_cli/commands/config.py Registers new keys for apm config set/get and includes them in listing output and valid-keys messaging.
tests/unit/test_config_command.py Adds unit tests for new config keys, CLI routing, and listing/valid-keys behavior.
tests/unit/test_protocol_config_precedence.py Adds precedence-focused tests for CLI/env/config/default resolution and round-trip config persistence.
docs/src/content/docs/reference/environment-variables.md Documents new transport env vars and how they relate to persisted apm config keys.
docs/src/content/docs/reference/cli/config.md Documents new config keys, their precedence, and usage examples.
Comments suppressed due to low confidence (3)

docs/src/content/docs/reference/environment-variables.md:41

  • Documentation mismatch: this table says APM_GIT_PROTOCOL only accepts 'ssh' and 'https', but the implementation also accepts 'http' (and ProtocolPreference.from_str maps 'http' to HTTPS). Either document 'http' as accepted (and clarify how it's treated), or reject it in get_apm_protocol_pref() for consistency.
| Variable | Purpose | Default | Notes |
|---|---|---|---|
| `APM_GIT_PROTOCOL` | Preferred clone protocol for shorthand (`owner/repo`) dependencies. Accepted values: `ssh`, `https`. | unset | Equivalent to `--ssh` / `--https` flag. Resolution: CLI flag → env var → `ssh` key in `~/.apm/config.json` → git `insteadOf` rules → HTTPS. |
| `APM_ALLOW_PROTOCOL_FALLBACK` | Set to `1` (or `true`/`yes`/`on`) to enable the legacy cross-protocol fallback chain. When enabled, a failed clone is retried with the opposite protocol. | unset | Equivalent to `--allow-protocol-fallback`. Resolution: CLI flag → env var → `allow-protocol-fallback` key in `~/.apm/config.json` → `false`. |

tests/unit/test_protocol_config_precedence.py:185

  • Non-ASCII Unicode right-arrow characters (U+2192) are used in this comment ('... -> fallthrough ...'), violating the printable-ASCII-only rule for source files. Replace with ASCII '->'.
    def test_unrecognised_env_val_falls_through_to_config(self):
        """An unrecognised APM_GIT_PROTOCOL value falls through to config."""
        import apm_cli.config as cfg_module

        with (
            patch.object(cfg_module, "get_ssh", return_value=True),
            patch.dict(os.environ, {"APM_GIT_PROTOCOL": "git"}),
        ):
            # 'git' is not in (ssh, https, http) → fallthrough to config (ssh=True)
            assert cfg_module.get_apm_protocol_pref() == "ssh"

docs/src/content/docs/reference/cli/config.md:73

  • This section includes non-ASCII punctuation: an em dash (U+2014) in the list item and a Unicode ellipsis (U+2026) in 'apm config set ...'. Replace with ASCII equivalents (e.g., '-' and '...') to comply with the printable-ASCII-only docs rule.
`allow-protocol-fallback` and `ssh` follow the layered transport precedence:

1. CLI flag (`--allow-protocol-fallback`, `--ssh`) — highest priority
2. Environment variable (`APM_ALLOW_PROTOCOL_FALLBACK=1`, `APM_GIT_PROTOCOL=ssh`)
3. Value in `~/.apm/config.json` (`apm config set …`)
4. Built-in default (`false` / no preference)

Comment thread src/apm_cli/config.py Outdated
Comment thread src/apm_cli/commands/config.py
Comment thread tests/unit/test_protocol_config_precedence.py Outdated
Comment thread docs/src/content/docs/reference/environment-variables.md
Comment thread docs/src/content/docs/reference/cli/config.md Outdated
Comment thread src/apm_cli/config.py Outdated
@sergio-sisternes-epam sergio-sisternes-epam added the panel-review Trigger the apm-review-panel gh-aw workflow label May 13, 2026
@github-actions
Copy link
Copy Markdown

.

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

  • feat: persist transport flags via apm config #1308 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
  • #1308 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #1308 · ● 4.5M ·

@github-actions
Copy link
Copy Markdown

APM Review Panel: needs_rework

feat: persist transport flags via apm config -- eliminates the daily --ssh repetition for SSH-only and corporate users; two functional gaps (ghost call-sites, broken unset) need resolution before the feature delivers on its full promise.

cc @Aaryan-Dadu @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

This PR targets a concrete, well-documented pain point: users in SSH-only and corporate GHES environments who must re-pass --ssh on every apm install invocation. The layered resolution design (CLI flag > env var > config > default) is architecturally consistent with APM's existing get_apm_* helper pattern, the docs are accurate and well-placed, and the test surface for the new precedence logic is solid. The panel is broadly positive on direction and scope.

Two functional gaps demand attention before this ships as-is. First, the auth-expert identified that github_downloader.py and validation.py retain direct calls to protocol_pref_from_env() / is_fallback_allowed(), bypassing the new config layer entirely. A user who runs apm config set ssh true will get SSH transport from the install.py path but revert to env-only semantics on any code path that constructs GithubDownloader without explicit args or that calls validation.py:317 directly. This is a user-visible correctness break: the config key appears to work but silently fails on a subset of calls. The test-coverage-expert independently confirmed no integration-tier test guards the full config-to-install wiring, which means neither the ghost call-site gap nor the end-to-end user promise "set once, install forever" is defended automatically. Second, the test-coverage-expert verified (evidence: outcome=missing, severity=recommended) that apm config unset allow-protocol-fallback and apm config unset ssh fall through to logger.error / sys.exit(1) because no unset branch exists for these keys. This is a silent, undocumented failure that breaks a standard CLI contract users rely on.

The remaining panel signals -- env-var constant duplication (python-architect + supply-chain, converged), http aliasing undocumented (supply-chain, auth, doc-writer, test-coverage, converged), apm config get showing stored not effective value (cli-logging), ssh key naming (devx-ux), noise in apm config output (devx-ux + cli-logging, converged), and missing CHANGELOG entry (oss-growth) -- are all recommended follow-ups that do not break the happy path. The http alias issue carries a mild future-caller risk flagged by both the auth-expert and supply-chain-security-expert, but ProtocolPreference.from_str currently absorbs it safely; normalizing or documenting it is a low-cost hardening step. The ssh key naming question (prefer-ssh vs ssh) raised by devx-ux is the only area where a pre-merge decision is worth making: renaming after users have set ssh in dotfiles is a breaking change, while renaming now costs only a search-replace. The maintainer should make a deliberate call here rather than letting it ship by default.

Dissent. The devx-ux-expert and doc-writer mildly disagree on unset ergonomics: devx-ux calls apm config set ssh false non-idiomatic and prefers unset, while doc-writer notes that unset is currently unimplemented and suggests documenting set false as the workaround. The test-coverage-expert's evidence confirms unset is broken (exit 1), so devx-ux wins on principle but doc-writer's workaround is the pragmatic near-term answer -- implement unset properly and then document it as idiomatic.

Aligned with: Portable by manifest (config-layer persistence means transport preference travels with the user's environment rather than living only in shell aliases), Secure by default (both keys default to false; no transport downgrade occurs without explicit opt-in), Governed by policy (layered resolution gives platform teams a clean env-var override surface enforceable centrally), Pragmatic as npm (npm's prefer-* convention for protocol keys signals protocol preference over capability toggle -- the bare ssh key reads ambiguously).

Growth signal. "Set once, install forever" is the punchy, tweetable story for SSH-only and GHES corporate users -- a high-value segment that APM has not yet explicitly targeted in release communications. The missing CHANGELOG entry is the single highest-leverage gap: the prior SSH/transport feature (#778) set a precedent for a detailed changelog line that became the reference for downstream narrative. This PR should get an equivalent entry under [Unreleased] before it ships so the release post can lead with this quality-of-life beat and the FAQ entry ("Do I need to pass --ssh every time? No, run apm config set ssh true once") can be grounded in a specific version.

Panel summary

Persona B R N Takeaway
Python Architect 0 1 2 Clean, well-scoped feature; env-var constant duplication is the only compounding architectural debt.
CLI Logging Expert 0 1 3 New keys integrate cleanly; apm config get shows stored, not effective, value -- debugging friction for the target user segment.
DevX UX Expert 0 3 2 Solves a real pain point; key name ssh is ambiguous pre-merge decision; unconditional default-false display adds noise.
Supply Chain Security Expert 0 2 1 No blocking supply-chain issues; env-var constant duplication risks silent name-drift; http alias undocumented.
OSS Growth Hacker 0 1 2 Genuine friction-killer for SSH-only and corporate users; missing CHANGELOG entry is the highest-leverage gap before ship.
Auth Expert 0 2 1 Config layer correctly wired in install.py; ghost call-sites in github_downloader.py and validation.py bypass it entirely.
Doc Writer 0 1 4 Docs accurate and well-structured; APM_GIT_PROTOCOL Notes column omits http as accepted alias.
Test Coverage Expert 0 2 2 Install pipeline config-layer wiring has no integration test; apm config unset for new keys silently exits 1.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

  1. [Auth Expert] Ghost call-sites in github_downloader.py and validation.py bypass the new config layer and silently revert to env-only transport semantics. -- Users who set ssh=true in config will get inconsistent transport behavior depending on which internal code path is taken; this is the most user-visible correctness gap in the PR and the only one that could cause silent mis-behavior post-merge.

  2. [Test Coverage Expert] apm config unset allow-protocol-fallback and apm config unset ssh silently exit 1 -- the unset command handler has no branch for these keys and no backend implementation. -- Shipping a config key that cannot be unset without hand-editing JSON breaks a standard CLI contract and will generate support noise.

  3. [Test Coverage Expert] No integration-tier test validates the full config-to-install wiring -- the user promise "apm config set ssh true causes subsequent installs to use SSH" is unguarded at the integration-with-fixtures tier. -- Unit mocks validate the helper in isolation but cannot catch the ghost call-site regression or any future refactor that breaks the wiring.

  4. [OSS Growth Hacker] No CHANGELOG entry exists for this feature under [Unreleased]. -- The prior SSH/transport feature (Transport Selection v1: honor user-specified transport, no silent downgrade #778) set the changelog precedent that feeds release narrative and the FAQ; missing this entry means the feature ships silently and the existing narrative for Transport Selection v1: honor user-specified transport, no silent downgrade #778 feels incomplete.

  5. [DevX UX Expert] Config key ssh is ambiguous -- prefer prefer-ssh to match npm conventions and signal protocol preference rather than capability toggle. -- Renaming after users have set ssh in dotfiles is a breaking change; a deliberate decision now costs only a search-replace, deferring forecloses the option.

Architecture

classDiagram
    direction LR

    class config {
        <<Module>>
        +get_allow_protocol_fallback() bool
        +set_allow_protocol_fallback(enabled) None
        +get_ssh() bool
        +set_ssh(enabled) None
        +get_apm_allow_protocol_fallback() bool
        +get_apm_protocol_pref() str|None
        +get_config() dict
        +update_config(patch) None
    }
    note for config "Resolution helpers follow env > config > default"

    class transport_selection {
        <<Module>>
        +ENV_PROTOCOL: str
        +ENV_ALLOW_FALLBACK: str
        +protocol_pref_from_env() ProtocolPreference
        +is_fallback_allowed() bool
    }
    note for transport_selection "Declares same env-var names as config.py"

    class ProtocolPreference {
        <<Enum>>
        NONE
        SSH
        HTTPS
        +from_str(value) ProtocolPreference
    }

    class install_cmd {
        <<Module>>
        +install(...)
    }

    class config_cmd {
        <<Module>>
        +VALID_KEYS: dict
        +cmd_config_set(...)
        +cmd_config_get(...)
    }

    install_cmd ..> config : "get_apm_protocol_pref()\nget_apm_allow_protocol_fallback()"
    install_cmd ..> transport_selection : "ProtocolPreference"
    transport_selection *-- ProtocolPreference : defines
    config_cmd ..> config : "set_allow_protocol_fallback()\nset_ssh()"

    class config:::touched
    class install_cmd:::touched
    class config_cmd:::touched
    classDef touched fill:#fff3b0,stroke:#d47600
Loading
flowchart TD
    A([apm install --ssh / --allow-protocol-fallback / neither]) --> B{CLI flag set?}
    B -- yes --> C[ProtocolPreference.SSH / .HTTPS CLI flag wins]
    B -- no --> D[config.get_apm_protocol_pref]
    D --> E{APM_GIT_PROTOCOL env set?}
    E -- ssh/https/http --> F[return env value]
    E -- not set --> G{config.get_ssh from config.json}
    G -- true --> H[return ssh]
    G -- false --> I[return None - ProtocolPreference.NONE]
    F --> J[ProtocolPreference.from_str]
    H --> J
    I --> J
    C --> K{allow_protocol_fallback CLI flag set?}
    J --> K
    K -- no --> L[config.get_apm_allow_protocol_fallback]
    L --> M{APM_ALLOW_PROTOCOL_FALLBACK env set?}
    M -- 1/true/yes/on --> N[allow_protocol_fallback = True]
    M -- not set --> O{config allow_protocol_fallback key}
    O -- true --> N
    O -- false --> P[allow_protocol_fallback = False]
    K -- yes --> N
    N --> Q[TransportSelector.select]
    P --> Q
Loading
sequenceDiagram
    actor User
    participant CLI as apm install
    participant Config as config.py
    participant FS as ~/.apm/config.json
    participant TS as TransportSelector

    User->>CLI: apm config set ssh true
    CLI->>Config: set_ssh(True)
    Config->>FS: write {"ssh": true}

    User->>CLI: apm install pkg
    CLI->>Config: get_apm_protocol_pref()
    Config->>FS: read config
    FS-->>Config: {"ssh": true}
    Config-->>CLI: "ssh"
    CLI->>TS: ProtocolPreference.from_str("ssh") - SSH
    CLI->>Config: get_apm_allow_protocol_fallback()
    Config-->>CLI: False
    CLI->>TS: select(pref=SSH, fallback=False)
    TS-->>CLI: git clone via SSH
Loading

Recommendation

Address the two functional gaps -- ghost call-sites in github_downloader.py / validation.py and the broken apm config unset handler -- before this merges. Both are straightforward fixes: wire the config helpers into GithubDownloader's default-arg resolution path (mirroring the install.py change), add unset_ssh / unset_allow_protocol_fallback functions to config.py, and add the corresponding unset branches to the config command handler. Add a CHANGELOG entry under [Unreleased] in the same pass. Once those three items land, the maintainer should make a deliberate call on the ssh vs prefer-ssh naming question before merge -- that is the one decision that cannot be cleanly deferred. The remaining panel findings (integration test, effective-value display in apm config get, http alias doc, constant extraction, output noise) are solid post-merge follow-up material.


Full per-persona findings

Python Architect

  • [recommended] Env-var name constants are duplicated across two modules -- extract to a shared leaf at src/apm_cli/config.py:9
    APM_ALLOW_PROTOCOL_FALLBACK and APM_GIT_PROTOCOL are declared as ENV_ALLOW_FALLBACK / ENV_PROTOCOL in transport_selection.py AND as _ENV_ALLOW_PROTOCOL_FALLBACK / _ENV_GIT_PROTOCOL in config.py. If either set is renamed, the two modules silently read different environment variables with no compiler or test catching the divergence. Fix: a zero-dependency src/apm_cli/deps/_env_keys.py (no imports) lets both modules import safely without a circular-import cycle.
    Suggested: Create src/apm_cli/deps/_env_keys.py with ENV_PROTOCOL = "APM_GIT_PROTOCOL" and ENV_ALLOW_FALLBACK = "APM_ALLOW_PROTOCOL_FALLBACK", then import from it in both modules.

  • [nit] get_apm_protocol_pref() can leak the raw string "http" -- normalize to "https" at src/apm_cli/config.py:220
    Docstring promises "ssh", "https", or None. APM_GIT_PROTOCOL=http returns raw "http". ProtocolPreference.from_str absorbs it correctly today, but any future caller pattern-matching on the return value will silently mis-handle it.
    Suggested: return "https" if env_val == "http" else env_val

  • [nit] Two unjustified function-body lazy imports in install.py:1307
    config.py is a leaf module with no circular-import risk. The underscore-alias inline import pattern signals "circular import workaround" -- using it without a cycle inverts the convention's meaning for future readers.

CLI Logging Expert

  • [recommended] apm config get / show surfaces stored value, not effective value at src/apm_cli/commands/config.py
    get_allow_protocol_fallback() / get_ssh() are called for display. If a user has APM_ALLOW_PROTOCOL_FALLBACK=1 in their shell but false in config.json, apm config get allow-protocol-fallback prints False while apm install honours True. Call the env-resolving wrappers (get_apm_allow_protocol_fallback / get_apm_protocol_pref) for display, with source annotation when the env var wins.

  • [nit] Fallback-path labels use raw kebab-case key, inconsistent with surrounding labels
    Surrounding fallback lines use Title Case (APM CLI Version, Temp Directory). New lines emit allow-protocol-fallback: and ssh: as raw keys.

  • [nit] Both new keys shown unconditionally at default False, adding noise to every user's output
    Follow the temp-dir pattern: only show when the value is non-default. apm config get <key> can still show the explicit value on direct request.

  • [nit] Python str(bool) renders True/False; convention expects true/false
    Config values are documented as lowercase booleans in the docs table. Use str(value).lower() at output sites.

DevX UX Expert

  • [recommended] Config key ssh is ambiguous -- prefer prefer-ssh at src/apm_cli/commands/config.py
    The bare key ssh reads as a feature toggle. npm uses the prefer-* pattern for protocol preference keys (prefer-offline, prefer-dedupe). prefer-ssh maps immediately onto the mental model. Renaming after users have set ssh in dotfiles is a breaking change.
    Suggested: Rename config key to prefer-ssh (internal: prefer_ssh). Keep --ssh as the CLI flag unchanged.

  • [recommended] apm config always prints both keys at False, adding noise to every user's output
    Violates "quiet on the happy path" -- noise for the 95% of users who have never changed these settings. Follow the temp-dir pattern.

  • [recommended] SSH auth error path doesn't hint at apm config set ssh true at src/apm_cli/commands/install.py
    User who encounters Permission denied (publickey) or HTTPS clone failure has no way to discover the config key without reading docs. npm prints actionable hints at the point of failure.
    Suggested: Add a transport-error hint in the clone failure branch pointing at apm config set ssh true.

  • [nit] Docs example resets preference with set ssh false instead of unset at docs/src/content/docs/reference/cli/config.md
    apm config unset ssh is idiomatic; set false leaves an explicit false in config.json while unset removes the entry entirely.

  • [nit] allow-protocol-fallback called "legacy" in PR body but docs don't signal deprecation
    If genuinely legacy, add a deprecation note so users know to prefer the explicit --ssh path going forward.

Supply Chain Security Expert

  • [recommended] Env-var constant duplication risks silent name-drift at src/apm_cli/config.py
    _ENV_ALLOW_PROTOCOL_FALLBACK and _ENV_GIT_PROTOCOL are copy-pasted strings in config.py (lines 11-12) while transport_selection.py declares ENV_ALLOW_FALLBACK and ENV_PROTOCOL with the same string values. No compile-time or test-time assertion ensures they stay in sync.
    Suggested: Add a standalone test asserting config._ENV_ALLOW_PROTOCOL_FALLBACK == transport_selection.ENV_ALLOW_FALLBACK, or refactor into a zero-dependency _env_keys.py module.

  • [recommended] get_apm_protocol_pref() accepts and returns "http" but docstring omits it at src/apm_cli/config.py:220
    ProtocolPreference.from_str maps "http" to HTTPS correctly today, but any future call-site using the raw string would open an unauthenticated HTTP clone path. Normalize "http" to "https" before returning, or drop it from the accepted set.

  • [nit] Raw config value for allow_protocol_fallback returned without bool-coercion at src/apm_cli/config.py:202
    A manually edited config.json with string "yes" or integer 1 activates the flag without going through the env-var truthy-set check. Wrap in bool() or validate on set_allow_protocol_fallback.

OSS Growth Hacker

Auth Expert

  • [recommended] Ghost call-sites bypass config layer: github_downloader.py and validation.py still call protocol_pref_from_env() / is_fallback_allowed() directly at src/apm_cli/deps/github_downloader.py
    GithubDownloader.__init__ falls back to protocol_pref_from_env() and is_fallback_allowed() when the caller passes protocol_pref=None / allow_fallback=None. These functions read only the env var -- they never consult ~/.apm/config.json. Any caller that constructs GithubDownloader without explicit values silently reverts to env-only semantics. validation.py:317 has the same gap for is_fallback_allowed().

  • [recommended] get_apm_protocol_pref() silently accepts "http" but docstring promises only "ssh"/"https" at src/apm_cli/config.py:220
    APM_GIT_PROTOCOL=http returns raw "http". In install.py this is immediately fed to ProtocolPreference.from_str("http") which maps it to ProtocolPreference.HTTPS -- no credentials travel over cleartext HTTP through this path. But if any future code uses the raw return value without passing it through from_str(), it could reach the _HTTP transport attempt. Reject "http" with a warning, or explicitly document and test the alias.

  • [nit] Late import inside function body for config helpers in install.py:1307 is unnecessary
    config.py is already imported at the module level in install.py; no circular-import risk justifies the inline pattern.

Doc Writer

  • [recommended] APM_GIT_PROTOCOL Notes column omits "http" as accepted value at docs/src/content/docs/reference/environment-variables.md
    The code accepts "http" and ProtocolPreference.from_str maps it to HTTPS. Change "Accepted values: ssh, https." to "Accepted values: ssh, https, http (alias for https)."

  • [nit] apm config unset section doesn't explicitly mention new keys are reset via set false
    A reader who tries apm config unset ssh will get an error with no explanation. Add: "This includes allow-protocol-fallback and ssh -- use apm config set allow-protocol-fallback false or apm config set ssh false to restore defaults."

  • [nit] allow-protocol-fallback example missing reset comment
    The ssh example includes # Reset to default: apm config set ssh false but the allow-protocol-fallback example does not. Add for consistency.

  • [nit] Table ordering -- transport booleans appear between path-type keys
    No change required for this PR. If the table grows, consider grouping by type.

  • [nit] Transport section placement in env-vars page is reasonable
    No change required. Placement after Authentication and before Registry is logical.

Test Coverage Expert

  • [recommended] apm config unset allow-protocol-fallback / apm config unset ssh silently fails with exit 1
    The unset command handler has explicit branches for temp-dir and copilot-cowork-skills-dir. allow-protocol-fallback and ssh have no branch, so apm config unset allow-protocol-fallback falls through to logger.error("Unknown configuration key") and sys.exit(1). No unset_allow_protocol_fallback or unset_ssh functions exist in config.py (grep confirmed). No test exercises runner.invoke(config, ["unset", "allow-protocol-fallback"]).
    Proof (missing at unit): tests/unit/test_config_command.py::TestConfigUnset::test_unset_allow_protocol_fallback_exits_0 -- proves: Running apm config unset allow-protocol-fallback succeeds with exit 0, not silently fails with exit 1 [devx]
    Suggested: Add unset_allow_protocol_fallback and unset_ssh to config.py, wire them in the unset handler, add tests.

  • [recommended] Install pipeline config-layer wiring has no integration test
    The diff in install.py replaces protocol_pref_from_env() / is_fallback_allowed() with config helpers. All tests are unit-level with patch.object mocks. tests/integration/test_transport_selection_integration.py exists but grep for config/set_ssh/get_apm_protocol_pref returned 0 hits.
    Proof (missing at integration-with-fixtures): tests/integration/test_transport_selection_integration.py::test_config_ssh_true_routes_install_to_ssh_transport -- proves: apm config set ssh true causes apm install to attempt SSH URLs -- the full config-to-install wiring is live, not just the helper in isolation [portability-by-manifest, devx]
    Suggested: Add fixture-based integration test: set_ssh(True) against isolated config file, invoke install command, assert attempted clone URL starts with git@ or `(redacted)

  • [nit] APM_GIT_PROTOCOL=http path is untested
    config.py:220 accepts "http". Grep of test files for APM_GIT_PROTOCOL.*http returned 0 hits.
    Proof (missing at unit): tests/unit/test_protocol_config_precedence.py::TestProtocolPrefPrecedence::test_env_var_http_returns_http_string

  • [nit] Precedence tests simulate CLI flag with Python variable, not real CLI invocation
    Tests like test_cli_flag_true_wins_over_everything set cli_flag = True and compute cli_flag or get_apm_allow_protocol_fallback(). Correctly tests the OR expression in install.py but does not verify Click wires the --allow-protocol-fallback option to the parameter.
    Proof (passed at unit): tests/unit/test_protocol_config_precedence.py::TestAllowProtocolFallbackPrecedence::test_cli_flag_true_wins_over_everything -- proves: OR expression short-circuits correctly when CLI flag is True [devx]

This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

  • feat: persist transport flags via apm config #1308 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
  • #1308 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #1308 · ● 4.5M ·

@github-actions github-actions Bot removed the panel-review Trigger the apm-review-panel gh-aw workflow label May 13, 2026
@Aaryan-Dadu
Copy link
Copy Markdown
Author

Aaryan-Dadu commented May 13, 2026

The copilot's panel-review flagged that the config key name ssh is ambiguous i.e., it reads as a capability toggle rather than a preference signal. prefer-ssh would align with npm conventions. So whichever will be preferred I can incorporate that.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.

Comments suppressed due to low confidence (2)

tests/unit/test_protocol_config_precedence.py:194

  • This comment contains a non-ASCII Unicode arrow. Repo encoding guidance requires printable ASCII in source files; replace with ASCII (e.g., '->').
            patch.dict(os.environ, {"APM_GIT_PROTOCOL": "git"}),
        ):
            # 'git' is not in (ssh, https, http) → fallthrough to config (ssh=True)
            assert cfg_module.get_apm_protocol_pref() == "ssh"

docs/src/content/docs/reference/cli/config.md:73

  • This section introduces non-ASCII characters (an em dash and a Unicode ellipsis in apm config set ...). Repo encoding guidance requires printable ASCII in docs; replace with ASCII equivalents (e.g., '-' and '...').
`allow-protocol-fallback` and `ssh` follow the layered transport precedence:

1. CLI flag (`--allow-protocol-fallback`, `--ssh`) — highest priority
2. Environment variable (`APM_ALLOW_PROTOCOL_FALLBACK=1`, `APM_GIT_PROTOCOL=ssh`)
3. Value in `~/.apm/config.json` (`apm config set …`)
4. Built-in default (`false` / no preference)

Comment thread src/apm_cli/config.py
@@ -4,6 +4,13 @@
import os
from typing import Optional # noqa: F401
Comment on lines 313 to 314
# ``APM_ALLOW_PROTOCOL_FALLBACK=1`` env var (the same escape-hatch
# the clone path honors) restores the legacy permissive chain.
# ``apm config set ssh true`` is honoured even when the downloader
# is constructed without explicit args (e.g. in validation.py).
from ..config import get_apm_protocol_pref as _get_pref
from .transport_selection import ProtocolPreference
Comment thread CHANGELOG.md
Comment on lines +10 to +13
### Added

- `apm config set ssh true` / `apm config set allow-protocol-fallback true` persist transport preferences to `~/.apm/config.json` so SSH-only and corporate GHES users no longer need to re-pass `--ssh` / `--allow-protocol-fallback` (or export env vars in shell profiles) on every `apm install`. Resolution order: CLI flag > `APM_GIT_PROTOCOL` / `APM_ALLOW_PROTOCOL_FALLBACK` env var > `apm config` > built-in default. `apm config unset ssh` and `apm config unset allow-protocol-fallback` remove the persisted value. (#1243)

Comment on lines +19 to +25
def _make_mock_protocol_preference():
"""Return a fresh ProtocolPreference-like enum from the real module."""
from apm_cli.deps.transport_selection import ProtocolPreference

return ProtocolPreference


Comment thread src/apm_cli/config.py
Comment on lines +269 to +272
Resolution order:
1. ``APM_GIT_PROTOCOL`` environment variable
(``"ssh"``, ``"https"``, or ``"http"`` — ``"http"`` is treated
as an alias for ``"https"`` by the transport selector)
Comment on lines +36 to +41
Controls how APM clones packages from Git hosts. These settings can also be persisted via [`apm config set`](./cli/config/) to avoid repeating flags or environment-variable exports.

| Variable | Purpose | Default | Notes |
|---|---|---|---|
| `APM_GIT_PROTOCOL` | Preferred clone protocol for shorthand (`owner/repo`) dependencies. Accepted values: `ssh`, `https`. | unset | Equivalent to `--ssh` / `--https` flag. Resolution: CLI flag → env var → `ssh` key in `~/.apm/config.json` → git `insteadOf` rules → HTTPS. |
| `APM_ALLOW_PROTOCOL_FALLBACK` | Set to `1` (or `true`/`yes`/`on`) to enable the legacy cross-protocol fallback chain. When enabled, a failed clone is retried with the opposite protocol. | unset | Equivalent to `--allow-protocol-fallback`. Resolution: CLI flag → env var → `allow-protocol-fallback` key in `~/.apm/config.json` → `false`. |
Comment on lines +55 to +57
| `temp-dir` | path | system temp | Directory used for clone and download operations. Useful when the OS temp directory is locked down (for example, corporate Windows endpoints rejecting `%TEMP%` with `[WinError 5]`). |
| `allow-protocol-fallback` | boolean | `false` | Enable the legacy cross-protocol fallback chain. When true, APM retries a failed clone with the opposite protocol (SSH→HTTPS or HTTPS→SSH). Equivalent to `--allow-protocol-fallback` or `APM_ALLOW_PROTOCOL_FALLBACK=1`. |
| `ssh` | boolean | `false` | Prefer SSH transport for shorthand (`owner/repo`) dependencies. Equivalent to `--ssh` or `APM_GIT_PROTOCOL=ssh`. |
Comment on lines +137 to +141
):
os.environ.pop("APM_GIT_PROTOCOL", None)
# CLI flag path: use_ssh=True → ProtocolPreference.SSH without calling helper
# Just ensure the helper returns None here (not ssh)
assert cfg_module.get_apm_protocol_pref() is None
Comment on lines 60 to 67
def _valid_config_keys() -> str:
"""Return valid config keys for messages."""
from ..core.experimental import is_enabled

keys = ["auto-integrate", "temp-dir"]
keys = ["auto-integrate", "temp-dir", "allow-protocol-fallback", "ssh"]
if is_enabled("copilot_cowork"):
keys.append("copilot-cowork-skills-dir")
return ", ".join(keys)
Copy link
Copy Markdown
Collaborator

@danielmeppiel danielmeppiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename key to prefer-ssh - thank you!

@Aaryan-Dadu
Copy link
Copy Markdown
Author

Please rename key to prefer-ssh - thank you!

I have made the changes @danielmeppiel but the ruff checks are failing due to changes unrelated to this PR.

image

@Aaryan-Dadu Aaryan-Dadu requested a review from danielmeppiel May 14, 2026 17:03
@danielmeppiel danielmeppiel added the panel-review Trigger the apm-review-panel gh-aw workflow label May 15, 2026
@github-actions
Copy link
Copy Markdown

APM Review Panel: ship_with_followups

PR #1308 lets SSH-first teams run 'apm config set prefer-ssh true' once and never pass --ssh again; two doc correctness bugs must be fixed before merge.

cc @danielmeppiel -- a fresh advisory pass is ready for your review.

This PR delivers a clean config-layer for transport persistence: env vars retain precedence, config file sits beneath them, and code defaults remain unchanged. The layering is sound and the auth-expert confirms no protocol-bypass regressions. The design is directionally correct and the user payoff is concrete. However, two documentation correctness bugs flagged by doc-writer are load-bearing: environment-variables.md names the config key as 'ssh' when it is actually 'prefer-ssh', meaning any user who hand-edits config.json following the docs will silently set an unrecognised key and get no effect. The second bug -- config.md line 48 contradicting its own examples on whether booleans are valid unset targets -- creates a support burden. Both must be fixed before this ships.

The devx-ux-expert raised the stale _CONFIG_KEY_DISPLAY_NAMES entry as blocking, but supply-chain-security-expert and cli-logging-expert independently classified it as nit/recommended dead code. The map is defined but not consumed by any live code path, so the practical blast radius is zero; fixing the stale 'ssh' entry is still correct hygiene but does not gate shipment. The two missing integration tests (test-coverage-expert, both 'missing' on an install-pipeline surface) are meaningful gaps: the config-file branch of allow-protocol-fallback through _validate_package_exists and the constructor config-aware default both lack fixtures-level coverage. These should be filed as follow-up issues, not treated as blockers, because the unit-level mock coverage is present and the feature logic is correct.

The supply-chain-security finding on config directory permissions (os.makedirs without mode=0o700) is the one finding with a genuine security implication on multi-user systems. It is classified recommended rather than blocking by the panelist, and no auth-expert objection was raised, but the fix is a one-liner and should be bundled in this PR rather than deferred. The CI/shared-home warning on allow-protocol-fallback and the FALLBACK_HINT update are similarly low-cost hardening steps worth landing now.

Dissent. devx-ux-expert classified the stale _CONFIG_KEY_DISPLAY_NAMES entry ("ssh": "ssh") as blocking; cli-logging-expert classified it recommended; supply-chain-security-expert classified it nit. CEO sides with cli-logging-expert and supply-chain-security-expert: the dict is dead code with no consumer, so its staleness has no runtime impact. The correct key ("prefer_ssh": "prefer-ssh") should be inserted and the stale entry removed, but this is not a ship-gate. Separately, doc-writer raised two blocking doc findings; no other panelist disputed them and CEO affirms them at blocking weight -- the env-vars.md wrong key name is a direct user-harm correctness bug.

Aligned with: Secure by default -- defaults are False for both new flags; the config directory permissions gap (os.makedirs without mode restriction) slightly undermines this on multi-user hosts and the one-liner fix should land in this PR. Pragmatic as npm -- persistent config flags reduce per-invocation friction for SSH-first and fallback-dependent teams; this is exactly the "set once, forget" ergonomics APM should offer.

Growth signal. OSS growth hacker identifies this as a launch-beat candidate for the enterprise/SSH cohort: 'APM now remembers your SSH preference.' The concrete hook -- 'SSH-first teams can run apm config set prefer-ssh true once and drop --ssh from every install' -- should lead the CHANGELOG [Unreleased] entry and any release-announcement copy. Long-term signal: apm init detecting SSH agent presence and offering to set prefer-ssh true during onboarding would close the discovery gap flagged in installation.md.

Panel summary

Persona B R N Takeaway
Python Architect 0 2 2 Solid env>config>default layering; stale display-name entry, duplicated env-var strings, and repeated unset boilerplate are the three concrete clean-ups worth making before the module grows further.
CLI Logging Expert 0 3 2 Two recommended fixes (always-on bool display adds noise; Python-capitalised True/False breaks CLI convention) plus one stale dead-code entry to remove; fallback-path label inconsistency is a nit.
DevX UX Expert 1 3 1 One correctness bug in the display-name map, one doc contradiction on unset, one stale env-var resolution string, and a protocol asymmetry gap that will confuse HTTPS-preferring users.
Supply Chain Security Expert 0 3 1 Defaults are secure (False); three recommended hardening gaps: config dir world-readable, stale FALLBACK_HINT, and silent CI persistence risk. One dead-code nit. No blocking issues.
OSS Growth Hacker 0 2 2 Solid friction-reducer for SSH/enterprise users; authentication.md still shows the old env-var escape hatch without mentioning the new persistent config alternative -- a quiet conversion miss.
Auth Expert 0 1 1 No auth bypasses or security regressions; config-aware transport resolution is consistent with AuthResolver initialization and env-var precedence is preserved.
Doc Writer 2 1 1 One blocking correctness bug (wrong config key name in env-var doc), one internal contradiction in config.md's unset description vs examples, one CHANGELOG structure issue, and a nit.
Test Coverage Expert 0 2 1 Precedence-chain unit coverage is solid; the persisted-config branch of the install-pipeline integration test is missing at the floor tier.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

  1. [Doc Writer] (blocking-severity) Fix environment-variables.md: change 'ssh key' to 'prefer-ssh key' in APM_GIT_PROTOCOL resolution chain -- Users who hand-edit config.json following the current docs will silently set an unrecognised key and see no effect. Direct user-harm correctness bug; should be fixed before merge.
  2. [Doc Writer] (blocking-severity) Fix config.md line 48: unset description must include prefer-ssh and allow-protocol-fallback as valid targets -- The current text contradicts the code examples two pages later. A user reading the description will believe unset does not work for boolean keys and reach for the wrong workaround.
  3. [Supply Chain Security Expert] Set mode=0o700 in os.makedirs and chmod 0o600 on config.json after initial creation -- One-liner fix; world-readable config on a multi-user host leaks allow_protocol_fallback state. Low effort, real hardening.
  4. [Test Coverage Expert] Add integration-with-fixtures tests for (1) config-file path of allow-protocol-fallback through _validate_package_exists and (2) constructor config-aware SSH default -- Both evidence blocks returned 'missing' on an install-pipeline surface. Unit mocks cover the logic but not the wiring; a future refactor could silently break either path.
  5. [CLI Logging Expert] Suppress False rows in 'config show' and render booleans as lowercase true/false -- Always-visible False rows add noise for users who never configured transport. Python-capitalised True/False breaks CLI convention. Both are low-effort polish fixes that reinforce the pragmatic-as-npm positioning.

Architecture

classDiagram
    direction TB

    class config_module {
        <<Module>>
        +get_apm_allow_protocol_fallback() bool
        +get_apm_protocol_pref() str|None
        +get_allow_protocol_fallback() bool
        +set_allow_protocol_fallback(bool)
        +unset_allow_protocol_fallback()
        +get_prefer_ssh() bool
        +set_prefer_ssh(bool)
        +unset_prefer_ssh()
        -_ENV_ALLOW_PROTOCOL_FALLBACK str
        -_ENV_GIT_PROTOCOL str
    }
    note for config_module "env > config > default resolver\nenv-var strings duplicated from transport_selection"

    class transport_selection_module {
        <<Module>>
        +is_fallback_allowed(cli_flag, env) bool
        +protocol_pref_from_env(env) ProtocolPreference
        +ENV_ALLOW_FALLBACK str
        +ENV_PROTOCOL str
    }
    note for transport_selection_module "ENV_ALLOW_FALLBACK / ENV_PROTOCOL\nduplicate config_module private constants"

    class TransportSelector {
        <<PureDecisionEngine>>
        +select(host, pref, allow_fallback) TransportPlan
    }

    class GitHubPackageDownloader {
        <<Facade>>
        -_protocol_pref ProtocolPreference
        -_allow_fallback bool
        -_transport_selector TransportSelector
        +__init__(auth_resolver, transport_selector, protocol_pref, allow_fallback)
    }
    note for GitHubPackageDownloader "Deferred imports of get_apm_protocol_pref\nand get_apm_allow_protocol_fallback in __init__\nwork around circular import"

    class commands_config_module {
        <<CLIGroup>>
        +set(key, value)
        +get(key)
        +unset(key)
        -_CONFIG_KEY_DISPLAY_NAMES dict
    }
    note for commands_config_module "_CONFIG_KEY_DISPLAY_NAMES has stale\n'ssh':'ssh' entry; missing 'prefer_ssh':'prefer-ssh'"

    class config_module:::touched
    class commands_config_module:::touched
    class GitHubPackageDownloader:::touched
    class transport_selection_module:::touched

    classDef touched fill:#fff3b0,stroke:#d47600

    config_module ..> transport_selection_module : duplicates env-var strings
    GitHubPackageDownloader *-- TransportSelector : owns
    GitHubPackageDownloader ..> config_module : deferred import in __init__
    commands_config_module ..> config_module : lazy imports via _get_config_setters/getters
Loading
flowchart TD
    A(["CLI: apm install / apm config set prefer-ssh true"]) --> B

    subgraph commands_config ["commands/config.py"]
        B["unset / set / get subcommand"] --> C["_get_config_setters() / _get_config_getters()\n[lazy import from config.py]"]
    end

    subgraph config_py ["config.py"]
        C --> D["set_prefer_ssh(bool) / set_allow_protocol_fallback(bool)\nupdate_config({key: val})\n[FS] writes ~/.apm/config.json"]
        E["get_apm_protocol_pref()"] --> F{"APM_GIT_PROTOCOL env set?"}
        F -- yes --> G["return env value\n(ssh/https/http)"]
        F -- no --> H{"get_prefer_ssh() == True?"}
        H -- yes --> I["return 'ssh'"]
        H -- no --> J["return None"]

        K["get_apm_allow_protocol_fallback()"] --> L{"_parse_allow_protocol_fallback_env\n(APM_ALLOW_PROTOCOL_FALLBACK) is not None?"}
        L -- yes --> M["return parsed bool"]
        L -- no --> N["get_allow_protocol_fallback()\n[FS] reads ~/.apm/config.json via get_config()"]
    end

    subgraph downloader ["deps/github_downloader.py"]
        O["GitHubPackageDownloader.__init__\nprotocol_pref=None, allow_fallback=None"] --> P["[deferred import] get_apm_protocol_pref()"]
        O --> Q["[deferred import] get_apm_allow_protocol_fallback()"]
        P --> E
        Q --> K
        P --> R["ProtocolPreference.from_str(pref_str)"]
        Q --> S["self._allow_fallback = bool"]
    end

    subgraph validation ["install/validation.py"]
        T["_validate_package_exists()"] --> U["get_apm_allow_protocol_fallback()"]
        U --> K
    end

    subgraph install_cmd ["commands/install.py"]
        V["_build_downloader() / install flow"] --> W["get_apm_protocol_pref()\nget_apm_allow_protocol_fallback()"]
        W --> E
        W --> K
    end
Loading

Recommendation

Ship after the two doc correctness bugs are resolved in this PR (wrong key name in environment-variables.md; unset description contradiction in config.md) and the config directory permissions one-liner is added. Everything else -- display-name dead-code cleanup, bool rendering, FALLBACK_HINT update, CI warning, integration test gaps -- should be filed as follow-up issues and linked from the PR description. The feature logic is sound, the security defaults are correct, and the user payoff is real. Do not let the accumulation of nits delay a change that directly unblocks SSH-first enterprise teams.


Full per-persona findings

Python Architect

  • [recommended] _ENV_ALLOW_PROTOCOL_FALLBACK / _ENV_GIT_PROTOCOL are declared twice -- once in config.py and once in transport_selection.py at src/apm_cli/config.py:11
    The comment says 'to avoid a circular import' but duplicating the string literals is a latent drift risk: if either side renames the constant the other side silently diverges. Canonical fix: extract both constants to a thin third module (e.g. apm_cli/deps/transport_constants.py) with no imports from either consumer.
    Suggested: Create src/apm_cli/deps/transport_constants.py containing the two constants; import from there in both config.py and transport_selection.py.

  • [recommended] Four unset_* functions in config.py repeat the same invalidate->load->del->write->invalidate pattern verbatim at src/apm_cli/config.py:136
    DRY threshold exceeded (4 call sites). A _unset_config_key(key: str) private helper eliminates all repetition and makes future bug fixes single-site.
    Suggested: Add def _unset_config_key(key: str) -> None: and delegate from all four unset_* functions.

  • [nit] _CONFIG_KEY_DISPLAY_NAMES still contains "ssh": "ssh" -- leftover from before the prefer_ssh rename at src/apm_cli/commands/config.py:24
    Dead entry, correct key should be "prefer_ssh": "prefer-ssh".
    Suggested: Replace "ssh": "ssh" with "prefer_ssh": "prefer-ssh".

  • [nit] In-constructor deferred imports in GitHubPackageDownloader.__init__ signal an unresolved layering tension at src/apm_cli/deps/github_downloader.py:188
    Workaround for circular import; resolves cleanly once constants are extracted to a shared module.

CLI Logging Expert

  • [recommended] Stale "ssh": "ssh" entry in _CONFIG_KEY_DISPLAY_NAMES should be removed at src/apm_cli/commands/config.py:24
    Maps to nothing used elsewhere and will silently diverge if the dict is ever iterated for validation or documentation generation.
    Suggested: Delete "ssh": "ssh". Add "prefer_ssh": "prefer-ssh" if the dict is used for normalisation.

  • [recommended] allow-protocol-fallback and prefer-ssh always appear in config show, even when at their defaults at src/apm_cli/commands/config.py:137
    temp-dir is guarded by if _temp_dir_val:. The new bools unconditionally emit rows showing False with no signal that these are defaults, not explicit choices. Violates signal-to-noise rule.
    Suggested: Mirror the temp-dir pattern: only emit rows when value deviates from the default (True).

  • [recommended] Booleans render as Python-capitalised True/False instead of lowercase true/false at src/apm_cli/commands/config.py:137
    CLI conventions (POSIX, YAML, JSON, every other config tool) use lowercase. Creates friction for users comparing output to documented values.
    Suggested: Use str(val).lower() or a _fmt_bool helper throughout.

  • [nit] Fallback click.echo path uses kebab-case key names for new entries but a human-readable label for temp-dir at src/apm_cli/commands/config.py:181
    Minor inconsistency in the fallback output path.
    Suggested: Use human-readable labels: ' Allow Protocol Fallback: ...' and ' Prefer SSH Transport: ...'.

  • [nit] apm config get prints False for unset booleans but Not set (using system default) for unset temp-dir at src/apm_cli/commands/config.py:315
    A user debugging the precedence chain cannot tell from this output which layer resolved the value.

DevX UX Expert

  • [blocking] _CONFIG_KEY_DISPLAY_NAMES still maps "ssh": "ssh" instead of "prefer_ssh": "prefer-ssh" at src/apm_cli/commands/config.py:24
    Internal config key was renamed but display-name table was not updated. Any code path that looks up the display name via the internal key will silently miss the entry, leaving prefer-ssh unlabelled or invisible in rendered output.
    Suggested: Change "ssh": "ssh" to "prefer_ssh": "prefer-ssh".

  • [recommended] Docs say unset only works for path keys; code supports it for both new boolean keys too at docs/src/content/docs/reference/cli/config.md:48
    config.md line 48 contradicts examples at lines 96-97 showing apm config unset prefer-ssh. Cannot both be correct.
    Suggested: Update the ### apm config unset KEY description to include allow-protocol-fallback and prefer-ssh as valid targets.

  • [recommended] Stale ssh key reference in environment-variables.md resolution chain at docs/src/content/docs/reference/environment-variables.md:40
    Line 40 describes APM_GIT_PROTOCOL resolution using ssh key in config.json, but the actual key is prefer_ssh. Users hand-editing config.json will write the wrong key and get silent no-op behaviour.
    Suggested: Replace 'ssh key in ~/.apm/config.json' with 'prefer_ssh key' or reference the CLI name.

  • [recommended] prefer-ssh boolean has no prefer-https counterpart; users who want to force HTTPS have no config escape hatch at docs/src/content/docs/reference/cli/config.md:57
    APM_GIT_PROTOCOL accepts both ssh and https, but config only controls SSH preference. No way to persist 'always use HTTPS' in config.
    Suggested: Add a note: 'To force HTTPS in all contexts, use APM_GIT_PROTOCOL=https; there is currently no config-file equivalent for pinning HTTPS.'

  • [nit] config get output prints Python-cased False/True instead of lowercase false/true at src/apm_cli/commands/config.py:181
    Suggested: Use str(val).lower() or explicit "true"/"false" strings.

Supply Chain Security Expert

  • [recommended] ~/.apm/ directory created without explicit mode restriction at src/apm_cli/config.py:23
    ensure_config_exists() calls os.makedirs(CONFIG_DIR) with no mode argument. On a multi-user system the directory inherits the process umask (commonly 755), making config.json world-readable. allow_protocol_fallback: true is a security-relevant state that other local users could observe or tamper with.
    Suggested: Use os.makedirs(CONFIG_DIR, mode=0o700, exist_ok=True) and os.chmod(CONFIG_FILE, 0o600) after initial creation.

  • [recommended] FALLBACK_HINT in transport_selection.py is now incomplete after this PR at src/apm_cli/deps/transport_selection.py:31
    The hint reads 'pass --allow-protocol-fallback or set APM_ALLOW_PROTOCOL_FALLBACK=1'. After this PR a third source exists: apm config set allow-protocol-fallback true. Users will be confused about which mechanism to use.
    Suggested: Update FALLBACK_HINT to include the config path. Add a note that the setting persists across sessions.

  • [recommended] No CI/shared-home warning when persisting allow_protocol_fallback: true at src/apm_cli/commands/config.py:254
    A user who runs apm config set allow-protocol-fallback true in a CI environment where $HOME is shared silently enables fallback for all subsequent installs. No warning at write time.
    Suggested: Emit logger.warning(...) when setting allow-protocol-fallback to True, noting that the setting persists and advising use of the env var in CI.

  • [nit] _CONFIG_KEY_DISPLAY_NAMES contains stale "ssh": "ssh" and is never read -- dead code at src/apm_cli/commands/config.py:24
    Either delete the dict or correct the entry to "prefer_ssh": "prefer-ssh" and wire it up.

OSS Growth Hacker

  • [recommended] authentication.md still teaches export APM_GIT_PROTOCOL=https without a forward-pointer to apm config set prefer-ssh false at docs/src/content/docs/getting-started/authentication.md
    Line 449 is the exact moment an SSH-blocked user is looking for the escape hatch. Showing only the env-var form leaves them one shell-profile edit away from forgetting about APM config -- a high-intent conversion miss.
    Suggested: After the export APM_GIT_PROTOCOL=https block, add: 'To persist this preference across sessions: apm config set prefer-ssh false'.

  • [recommended] CHANGELOG entry buries the user-facing hook inside a resolution-order explanation at CHANGELOG.md
    The lead sentence front-loads the mechanism rather than the payoff. 'SSH users: you never have to type --ssh again' is what gets copy-pasted into a Slack or tweet.
    Suggested: Rewrite the opening to lead with payoff: 'SSH-first teams can now run apm config set prefer-ssh true once and drop --ssh from every subsequent apm install.'

  • [nit] apm config unset prefer-ssh works but docs say only path keys are unsettable -- contradictory at docs/src/content/docs/reference/cli/config.md:48

  • [nit] No apm init or first-install touchpoint guides SSH-first users to discover prefer-ssh at docs/src/content/docs/getting-started/installation.md
    Suggested: Add a post-install callout: 'SSH users: run apm config set prefer-ssh true once to avoid passing --ssh on every install.'

Auth Expert

  • [recommended] Constructor docstring still says 'reads APM_GIT_PROTOCOL env' -- config layer is now consulted but not mentioned at src/apm_cli/deps/github_downloader.py:173
    Lines 173-176 describe the protocol_pref=None fallback as env-only. The implementation now resolves env -> config -> default. A caller reading only the docstring will not know that a persisted config value can override their expectations.
    Suggested: Update the docstring to mention ~/.apm/config.json as a resolution source.

  • [nit] allow_fallback_env variable name in validation.py is now misleading at src/apm_cli/install/validation.py:317
    Reads from config (env > config > False) but _env suffix implies env-only. Will confuse future readers tracing why fallback is enabled despite no env var being set.
    Suggested: Rename to allow_fallback or allow_fallback_cfg.

Doc Writer

  • [blocking] environment-variables.md names the config key as ssh, not prefer-ssh at docs/src/content/docs/reference/environment-variables.md:40
    Line 40 reads '-> ssh key in ~/.apm/config.json'. The actual CLI key is prefer-ssh. A user who runs apm config set ssh true will set an unrecognised key and get silent no-op behaviour. config.md correctly uses prefer-ssh throughout; env-vars.md contradicts it.
    Suggested: Change '-> ssh key in ~/.apm/config.json' to '-> prefer-ssh key in ~/.apm/config.json'.

  • [blocking] apm config unset description contradicts the examples for prefer-ssh at docs/src/content/docs/reference/cli/config.md:48
    Line 48 states 'Only temp-dir and copilot-cowork-skills-dir are unsettable; boolean keys are reset by set-ing them to their default.' Lines 96-97 show apm config unset prefer-ssh as valid. These cannot both be correct.
    Suggested: Update line 48: 'Boolean keys (prefer-ssh, allow-protocol-fallback) can be removed with unset; the built-in default (false) then applies. temp-dir and copilot-cowork-skills-dir are also unsettable.'

  • [recommended] CHANGELOG [Unreleased] contains two separate ### Added sections at CHANGELOG.md:21
    Keep a Changelog specifies one subsection per type per release. Having two ### Added headings will confuse changelog parsers and readers.
    Suggested: Merge the second ### Added block's bullet points into the first, then delete the duplicate heading.

  • [nit] env-vars.md transport section intro has explanatory padding that could be tightened at docs/src/content/docs/reference/environment-variables.md:36
    Suggested: 'These settings can also be persisted via apm config set.'

Test Coverage Expert

  • [recommended] No integration test exercises the config-file (not env-var) path of get_apm_allow_protocol_fallback through _validate_package_exists at tests/unit/install/test_validation_strict_transport.py
    The existing integration test covers the env-var branch (APM_ALLOW_PROTOCOL_FALLBACK=1). The persisted-config branch -- where env var is absent and config file has allow_protocol_fallback: true -- is only covered by unit-level mocks. Install-pipeline surface floor tier is integration-with-fixtures.
    Proof (missing): tests/unit/install/test_validation_strict_transport.py::test_config_persisted_allow_fallback_chains_both_schemes -- proves: When user has run apm config set allow-protocol-fallback true and no env var is set, _validate_package_exists still probes both transports [secure-by-default]

  • [recommended] Downloader constructor's config-aware default for protocol_pref has no integration test at tests/unit/deps/test_github_downloader_validation.py
    The new branch where the constructor is instantiated without explicit protocol_pref and config file says prefer_ssh: true has no integration-with-fixtures test. Grepped all of tests/ for get_apm_protocol_pref and get_apm_allow_protocol_fallback: only hits in the two new unit files.
    Proof (missing): tests/unit/deps/test_github_downloader_validation.py::test_constructor_reads_ssh_preference_from_config -- proves: When get_prefer_ssh returns True and no explicit protocol_pref arg is given, downloader selects SSH [devx]

  • [nit] test_protocol_config_precedence.py simulates CLI-flag path inline rather than calling real install dispatch at tests/unit/test_protocol_config_precedence.py
    Future refactors of install.py could silently break the wiring. Not blocking -- install invocation is covered elsewhere and the config-module unit tests are well-targeted.

This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

  • feat: persist transport flags via apm config #1308 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
  • #1308 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #1308 · ● 3.9M ·

@github-actions github-actions Bot removed the panel-review Trigger the apm-review-panel gh-aw workflow label May 15, 2026
Add allow-protocol-fallback and ssh as user-settable apm config keys so users can persist their preferred transport mode without re-typing CLI flags or modifying shell profiles on every session.

Precedence chain (highest to lowest):
  CLI flag > APM_ALLOW_PROTOCOL_FALLBACK/APM_GIT_PROTOCOL env var
           > apm config > built-in default
Changes:
- config.py: add get/set_allow_protocol_fallback(), get/set_ssh(), get_apm_allow_protocol_fallback() and get_apm_protocol_pref() resolution helpers that encode the full env > config > default chain
- commands/install.py: wire config into the transport-selection block;
- commands/config.py: register both keys in setters/getters/valid-keys, add them to all listing paths (rich table, plain-text, get all-keys)
- docs/reference/cli/config.md: new rows in keys table, resolution-order
  section for transport keys, usage examples
- docs/reference/environment-variables.md: new Transport and protocol section documenting APM_GIT_PROTOCOL and APM_ALLOW_PROTOCOL_FALLBACK
- tests: 74 new test cases across test_config_command.py and new test_protocol_config_precedence.py (149 pass, 0 fail)

Closes microsoft#1243
- Wire get_apm_allow_protocol_fallback / get_apm_protocol_pref into
  GitHubPackageDownloader defaults and validation.py so apm config
  keys are honoured on all code paths, not just install.py
- Add unset_allow_protocol_fallback() and unset_ssh() to config.py;
  register both in the unset command handler
- Add CHANGELOG entry under [Unreleased]
- Fix stale _CONFIG_KEY_DISPLAY_NAMES entry (ssh -> prefer_ssh)
- Lowercase bool output in apm config get (True/False -> true/false)
- Suppress false-default transport rows in apm config / apm config get
- Success message: "set to true/false" instead of "enabled/disabled"
- FALLBACK_HINT: mention apm config set as persistent alternative

config.py
- Add _unset_config_key(key) private helper; delegate all four
  unset_* functions through it (DRY, Python Architect recommended)

commands/config.py
- Emit logger.warning() when allow-protocol-fallback is persisted in
  a CI environment ($CI set) advising APM_ALLOW_PROTOCOL_FALLBACK=1
  for invocation-scoped use instead (Supply Chain Security recommended)

install/validation.py
- Rename allow_fallback_env -> allow_fallback; _env suffix was
  misleading after the value started resolving from config too
  (Auth Expert nit)

deps/github_downloader.py
- Update __init__ docstring for protocol_pref and allow_fallback to
  accurately describe env > config > default resolution instead of
  implying env-only lookups (Auth Expert recommended)

CHANGELOG.md
- Merge second ### Added block bullets into first; restore ### Fixed
  section; one subsection per type per release per Keep a Changelog
  (Doc Writer recommended)

docs/getting-started/authentication.md
- Add forward-pointer to "apm config set prefer-ssh false" after the
  APM_GIT_PROTOCOL=https escape-hatch block; highest-intent
  conversion point for the new config surface (OSS Growth Hacker
  recommended)
@Aaryan-Dadu
Copy link
Copy Markdown
Author

I've addressed all the blockers and nearly all the recommendations/nitpicks in the latest commits.

@danielmeppiel danielmeppiel added the panel-review Trigger the apm-review-panel gh-aw workflow label May 18, 2026
@github-actions
Copy link
Copy Markdown

APM Review Panel: ship_with_followups

PR #1308 ships clean config-layer SSH persistence; five follow-ups (one integration test, two doc fixes, one security escape hatch, one enterprise doc callout) should land before or shortly after.

cc @danielmeppiel -- a fresh advisory pass is ready for your review.

All eight panelists reviewed a coherent, well-scoped change. Auth expert confirmed the resolution order (CLI > env > config > default) is correctly implemented and AuthResolver is preserved. Python architect found the layering clean; the only structural smell is env-var constant duplication across two modules, which is a maintenance risk but not a correctness risk today. No panelist surfaced a blocking defect.

The highest-signal gap is from the test-coverage expert: no integration test exercises the config-file -> GitHubPackageDownloader wiring path. This is a 'secure-by-default' surface -- if the wiring silently breaks, SSH-only and GHES users who ran apm config set prefer-ssh true will fall back to HTTPS with no error. That promise must be guarded by an automated test. The supply-chain-security finding (no --no-allow-protocol-fallback escape hatch) is the second-highest signal: a user who persisted allow-protocol-fallback=true has no single-run CLI override other than an env var, which is a usability gap with mild security implications for regulated environments. Both deserve near-term follow-up but neither prevents shipping.

The OSS growth hacker surfaces a concrete adoption opportunity: GHES and regulated-industry users are the primary beneficiaries of this PR, yet enterprise docs (adoption-playbook.md) have zero mention of prefer-ssh. That is a missed conversion moment. The doc-writer found three fixable doc accuracy issues -- notably a factually wrong "all settable keys are unsettable" claim in config.md and a misleading apm config set prefer-ssh false snippet in authentication.md -- that should be patched promptly to avoid user confusion.

Dissent. Auth expert and python architect disagreed on lazy imports in github_downloader.py: auth-expert called them safe (zero circular import risk since config.py has no APM internal imports) and suggested moving to module level; python architect accepted them as acceptable but wanted inline comments. Given auth-expert's confirmation that the risk was already zero, the comment-only fix from python architect is sufficient and preferred over a structural move that could introduce regressions.

Aligned with: Portability-by-manifest (transport preference is user-scoped; no apm.yml change required); Secure-by-default (CLI flag supremacy is preserved -- except for the missing --no-allow-protocol-fallback escape hatch); Governed-by-policy (user-local config file, layered precedence design is extensible); Multi-harness/multi-host (SSH preference applies uniformly across GitHub.com and GHES; AuthResolver path preserved); OSS community-driven (addresses known GHES/SSH-only friction); Pragmatic-as-npm (apm config set prefer-ssh true is one command, persists, no per-invocation ceremony).

Growth signal. The OSS growth hacker is right: "one config command, never re-pass --ssh again" is a tweetable win and a concrete onboarding unlock for finance, healthcare, and air-gapped GHES segments. The enterprise docs gap is the highest-leverage amplification surface -- a single SSH callout paragraph in adoption-playbook.md converts readers already in the buying funnel. The CHANGELOG Highlights subsection suggestion is worth doing before the next release post; prefer-ssh persistence is the headline QoL win of this release and should not be buried.

Panel summary

Persona B R N Takeaway
Python Architect 0 2 2 Config layering is clean and correct; env-var constant duplication is the main structural smell; lazy imports in constructor are acceptable; base class removal is the right direction.
CLI Logging Expert 0 1 3 bare print() replaces _rich_warning() losing yellow color; double-space in [!] prefix is cosmetic noise; FALLBACK_HINT is good but surfaces in error body not as a formatted hint line.
DevX UX Expert 0 1 2 Solid feature with good docs; one discoverability gap in apm config get (no-arg) silently omitting transport keys when false, plus two nits.
Supply Chain Security Expert 0 1 2 PR is clean; one recommended fix: no CLI escape hatch to force-disable config-persisted allow-protocol-fallback for a single run.
OSS Growth Hacker 0 2 2 prefer-ssh persistence meaningfully reduces GHES/regulated-industry friction, but enterprise docs don't surface it -- the highest-value adopters will miss it.
Auth Expert 0 0 1 Config-aware resolution order (CLI > env > config > default) is correctly implemented; AuthResolver path preserved; no circular import risk; lockfile integrity fields intact.
Doc Writer 0 3 2 config.md and env-vars.md are accurate and well-structured; three fixable gaps: stale apm-guide commands.md, misleading auth.md snippet, and a false "all settable keys are unsettable" claim.
Test Coverage Expert 0 1 2 Unit coverage for config precedence and CLI get/set is solid; one integration-tier gap: no test exercises apm install with prefer-ssh sourced from ~/.apm/config.json; minor: no unset tests for new config keys.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

  1. [Test Coverage Expert] Add integration test: write prefer_ssh=true to temp config.json, construct GitHubPackageDownloader() with no explicit args, assert protocol_pref == ProtocolPreference.SSH -- Missing automated guard on the config->downloader wiring path; silent breakage would affect all GHES users who ran apm config set prefer-ssh true without triggering any error
  2. [Doc Writer] Fix config.md unset section: replace "All settable keys are unsettable" with an explicit list; fix authentication.md snippet to use apm config unset prefer-ssh instead of apm config set prefer-ssh false -- The config.md claim is factually wrong (auto-integrate cannot be unset); the auth.md snippet implies a no-op is meaningful -- both actively mislead users
  3. [Supply Chain Security Expert] Add --no-allow-protocol-fallback CLI flag or tri-state handling so a single apm install run can force-disable config-persisted allow-protocol-fallback -- Without an escape hatch, users in regulated environments who temporarily enabled fallback have no single-run override; env var workaround is not discoverable
  4. [OSS Growth Hacker] Add SSH-only environments callout (one paragraph + two-command snippet) to docs/src/content/docs/enterprise/adoption-playbook.md -- GHES users navigate to the enterprise section first; if prefer-ssh config is not surfaced there, the highest-value adopters miss the feature entirely
  5. [Doc Writer] Add prefer-ssh and allow-protocol-fallback to the config table in packages/apm-guide/.apm/skills/apm-usage/commands.md -- The apm-guide skill is consumed by AI assistants; omitting the new keys means agents will not surface transport-persistence config to users who ask

Architecture

classDiagram
    direction LR

    class GitHubPackageDownloader {
      <<IOBoundary>>
      +_protocol_pref ProtocolPreference
      +_allow_fallback bool
      +_transport_selector TransportSelector
      +auth_resolver AuthResolver
    }

    class TransportSelector {
      <<Strategy>>
      +select(pref, host) URL
    }

    class ProtocolPreference {
      <<ValueObject>>
      +SSH
      +HTTPS
      +INSTEADOF
      +from_str(s) ProtocolPreference
    }

    class ConfigModule {
      <<IOBoundary>>
      +get_apm_protocol_pref() str|None
      +get_apm_allow_protocol_fallback() bool
      +get_prefer_ssh() bool
      +get_allow_protocol_fallback() bool
    }

    class AuthResolver {
      <<Strategy>>
      +resolve_for(host) AuthContext
    }

    class AuthContext {
      <<ValueObject>>
      +token str
      +source str
    }

    GitHubPackageDownloader *-- TransportSelector : uses
    GitHubPackageDownloader *-- AuthResolver : uses
    GitHubPackageDownloader ..> ConfigModule : lazy-imports in __init__
    GitHubPackageDownloader ..> ProtocolPreference : stores
    TransportSelector ..> ProtocolPreference : reads
    AuthResolver ..> AuthContext : returns

    class ConfigModule:::touched

    classDef touched fill:#fff3b0,stroke:#d47600
Loading
flowchart TD
    A([apm install / apm config set]) --> B{CLI flag\nprotocol_pref / allow_fallback\npassed explicitly?}
    B -- yes --> C[Use flag value directly\nSkip env+config lookup]
    B -- no --> D{APM_GIT_PROTOCOL\nenv var set?}
    D -- ssh/https/http --> E[Return env var value\nconfig.get_apm_protocol_pref]
    D -- unset/empty --> F{prefer_ssh in\n~/.apm/config.json?}
    F -- True --> G[Return 'ssh']
    F -- False/absent --> H[Return None\nlet git insteadOf decide]
    C --> Z
    E --> Z
    G --> Z
    H --> Z
    Z[ProtocolPreference.from_str] --> ZZ[TransportSelector.select]

    AA{APM_ALLOW_PROTOCOL_FALLBACK\nenv var set?} --> BB{Truthy value?}
    BB -- 1/true/yes/on --> CC[Return True]
    BB -- 0/false/no/off --> DD[Return False]
    BB -- unrecognised/empty --> EE{allow_protocol_fallback\nin ~/.apm/config.json?}
    AA -- unset --> EE
    EE -- present --> FF[Return stored bool]
    EE -- absent --> GG[Return False default]
Loading
sequenceDiagram
    actor User
    participant CLI as apm install CLI
    participant DL as GitHubPackageDownloader
    participant Cfg as config.py
    participant Env as OS Env
    participant TS as TransportSelector
    participant Git as git clone

    User->>CLI: apm install (no --prefer-ssh flag)
    CLI->>DL: __init__(protocol_pref=None, allow_fallback=None)
    DL->>Cfg: get_apm_protocol_pref()  [lazy import]
    Cfg->>Env: os.environ.get('APM_GIT_PROTOCOL')
    Env-->>Cfg: '' (unset)
    Cfg->>Cfg: get_prefer_ssh() -> read ~/.apm/config.json
    Cfg-->>DL: 'ssh' or None
    DL->>Cfg: get_apm_allow_protocol_fallback()  [lazy import]
    Cfg->>Env: os.environ.get('APM_ALLOW_PROTOCOL_FALLBACK')
    Env-->>Cfg: None (unset)
    Cfg->>Cfg: get_allow_protocol_fallback() -> read config
    Cfg-->>DL: False
    DL->>TS: select(protocol_pref, host_url)
    TS-->>DL: resolved_url
    DL->>Git: clone resolved_url
Loading

Recommendation

The change is correct, well-layered, and addresses a real community friction point. Ship it. The five follow-ups above should be tracked as a post-ship batch: the integration test (follow-up 1) is the highest-signal item and should land in the next PR touching the transport stack; the two doc accuracy fixes (follow-up 2) should be patched promptly to avoid user confusion; the --no-allow-protocol-fallback escape hatch and the enterprise doc callout are high-leverage and belong in the next sprint.


Full per-persona findings

Python Architect

  • [recommended] ENV_PROTOCOL / ENV_ALLOW_FALLBACK constants are duplicated across transport_selection.py and config.py at src/apm_cli/deps/transport_selection.py
    transport_selection.py defines ENV_PROTOCOL = 'APM_GIT_PROTOCOL' and ENV_ALLOW_FALLBACK = 'APM_ALLOW_PROTOCOL_FALLBACK'. config.py re-declares them as _ENV_GIT_PROTOCOL and _ENV_ALLOW_PROTOCOL_FALLBACK with an inline comment acknowledging the duplication. Two sources of truth for the same string literals; a future rename or typo fix in one won't propagate to the other.
    Suggested: Extract to src/apm_cli/deps/transport_constants.py (no imports needed) and import from there in both modules. Eliminates duplication without reintroducing any circular import.

  • [recommended] get_apm_protocol_pref() cannot express an explicit HTTPS pin via config -- only SSH or None at src/apm_cli/config.py
    Config layer exposes prefer_ssh: bool, mapping True to 'ssh' and False/absent to None. No way to pin HTTPS via apm config set without the env var. Users wanting to force HTTPS (overriding insteadOf rules) must use an env var.
    Suggested: Either accept the asymmetry and document it in CLI help output, or introduce a string-valued prefer_protocol config key as a follow-on (keeping backward compat via migration in get_apm_protocol_pref).

  • [nit] Lazy imports inside __init__ body create invisible coupling at src/apm_cli/deps/github_downloader.py
    Suggested: Add inline comments: # lazy: breaks config <-> deps circular import and # lazy: breaks orchestrator <-> downloader circular import.

  • [nit] _config_cache global is not thread-safe at src/apm_cli/config.py
    Suggested: Add comment: # Not thread-safe; safe for single-threaded CLI use only.

CLI Logging Expert

  • [recommended] bare print() in base.py bypasses _rich_warning(), losing yellow coloring and framework consistency at src/apm_cli/adapters/client/base.py
    _rich_warning() renders in yellow via _rich_echo, making warnings visually distinct. Every other warning in the codebase uses _rich_warning(); this is the only outlier.
    Suggested: Replace print(f"[!] Warning: ...") with _rich_warning(...). Import _rich_warning at the top of the file.

  • [nit] Double-space after [!] in the print() format string is inconsistent with STATUS_SYMBOLS convention at src/apm_cli/adapters/client/base.py
    Suggested: Use single space: "[!] Warning: "

  • [nit] Stray 'm' prefix mentioned in PR summary is not present in committed code -- no action needed

  • [nit] FALLBACK_HINT is plain prose in error body, not a formatted [i] hint line at src/apm_cli/deps/bare_cache.py
    Suggested: After emitting the error, emit a separate _rich_info(plan.fallback_hint) line so the apm config set allow-protocol-fallback true command is visually distinct.

DevX UX Expert

  • [recommended] apm config get (no-arg) silently omits prefer-ssh and allow-protocol-fallback when false at src/apm_cli/commands/config.py
    A user running apm config get to audit settings will not see transport keys at all if at defaults. auto-integrate always appears; transport keys don't. Inconsistency causes confusion for GHES users verifying their setup.
    Suggested: In the no-arg apm config get branch, always print both transport keys with their effective value. The rich table display can keep hide-when-false; the dedicated get path should be exhaustive.

  • [nit] The allow-protocol-fallback table description calls the fallback chain 'legacy', confusing for users being guided to enable it at docs/src/content/docs/reference/cli/config.md
    Suggested: Drop 'legacy' unless there is a deprecation timeline to cite.

  • [nit] The unset success message parenthetical resolution hint is useful but missing from set at src/apm_cli/commands/config.py
    Suggested: After successful set, append active resolution tier to help users understand what they have overridden.

Supply Chain Security Expert

  • [recommended] No CLI escape hatch to override allow-protocol-fallback=true from config back to false for a single run at src/apm_cli/commands/install.py
    install.py uses allow_protocol_fallback = allow_protocol_fallback or _get_apm_apf(). If a user has persisted allow-protocol-fallback: true in config, there is no CLI flag to force-disable it for a single apm install. Only workaround is APM_ALLOW_PROTOCOL_FALLBACK=0, which is not discoverable.
    Suggested: Replace the or with a tri-state: if the CLI flag was explicitly passed (not just defaulted to False), honour it as authoritative. Or add --no-allow-protocol-fallback.

  • [nit] CI warning for allow-protocol-fallback is post-write, not pre-write in src/apm_cli/commands/config.py
    Suggested: Emit the CI warning before calling setter().

  • [nit] Protocol fallback does not re-verify repo identity after protocol swap -- but post-clone SHA pin check covers this
    Suggested: Consider logging the resolved remote URL after a fallback clone for audit trail.

OSS Growth Hacker

  • [recommended] Enterprise docs (adoption-playbook.md, index.md) have zero mention of prefer-ssh or SSH transport config at docs/src/content/docs/enterprise/adoption-playbook.md
    GHES users navigate to the enterprise section first. If apm config set prefer-ssh true is not surfaced there, most won't find it through the reference CLI page.
    Suggested: Add a 'SSH-only environments' callout with one paragraph and a two-command snippet. Cross-link to the config reference.

  • [recommended] CHANGELOG [Unreleased] leads with prefer-ssh (good) but it is visually buried under 6+ subsequent Added bullets at CHANGELOG.md
    Suggested: Consider a 'Highlights' subsection with 2-3 bullets (prefer-ssh persistence, MCP registry v0.1, apm pack --json).

  • [nit] The prefer-ssh CHANGELOG sentence is accurate but 85 words -- too long for release social post at CHANGELOG.md
    Suggested: Lead with: "SSH-only and GHES users: run apm config set prefer-ssh true once -- no more --ssh on every install."

  • [nit] No quickstart mention of SSH environments -- first-run GHES users hit HTTPS clone errors with no in-product nudge at docs/src/content/docs/getting-started/quick-start.md
    Suggested: Add a collapsible 'Corporate/GHES environment?' tip block near the first apm install step.

Auth Expert

  • [nit] Lazy imports inside __init__ are safe but unconventional at src/apm_cli/deps/github_downloader.py
    Since config.py has zero APM internal imports, the circular import risk was already zero. Auth expert confirms: resolution order correctly implemented, AuthResolver path preserved, lockfile integrity fields (package.source, package.resolved_commit) intact in the inlined validation code.
    Suggested: Move from ..config import get_apm_protocol_pref, get_apm_allow_protocol_fallback to module-level imports for clarity.

Doc Writer

  • [recommended] packages/apm-guide/commands.md config table omits prefer-ssh and allow-protocol-fallback at packages/apm-guide/.apm/skills/apm-usage/commands.md
    The apm-guide skill is consumed by AI assistants. The config table still lists only auto-integrate, temp-dir, and copilot-cowork-skills-dir. Agents using this skill will not know the new transport-persistence keys exist.
    Suggested: Add prefer-ssh and allow-protocol-fallback to the config get, set, and unset rows, mirroring the phrasing in reference/cli/config.md.

  • [recommended] authentication.md snippet recommends apm config set prefer-ssh false -- a no-op that adds confusion at docs/src/content/docs/getting-started/authentication.md
    The default of prefer-ssh is false, so set prefer-ssh false changes nothing. The comment "# explicit: never prefer SSH" implies it is meaningful, which is misleading.
    Suggested: Replace with: apm config unset prefer-ssh # revert a previously-set SSH preference

  • [recommended] config.md unset section claims "All settable keys are unsettable" but auto-integrate cannot be unset at docs/src/content/docs/reference/cli/config.md
    The unset handler has no branch for auto-integrate. apm config unset auto-integrate exits with "Unknown configuration key". The doc claim is factually wrong.
    Suggested: List explicitly the unsettable keys: temp-dir, copilot-cowork-skills-dir, prefer-ssh, allow-protocol-fallback. Note auto-integrate must be reset by setting it to its default.

  • [nit] CHANGELOG transport-config entry is accurate and placement in Added is correct; resolution-order sentence could be trimmed at CHANGELOG.md
    Suggested: Optional trim; acceptable as-is.

  • [nit] environment-variables.md new rows use arrow notation inconsistently with other rows at docs/src/content/docs/reference/environment-variables.md
    Suggested: Either convert to prose style or adopt arrows as the pattern for resolution-chain rows consistently.

Test Coverage Expert

  • [recommended] No integration-tier test exercises apm install with prefer-ssh sourced from config file (not CLI flag) at tests/integration/test_transport_selection_integration.py
    GitHubPackageDownloader.__init__ calls get_apm_protocol_pref() when no explicit arg is passed. Existing integration tests pass ProtocolPreference.SSH directly, bypassing the config-resolution path. Silent breakage would affect all GHES users who ran apm config set prefer-ssh true.
    Proof (missing at integration-with-fixtures): tests/integration/test_transport_selection_integration.py::test_prefer_ssh_from_config_wires_into_downloader -- proves: apm config set prefer-ssh true causes apm install to use SSH cloning without requiring --ssh CLI flag [secure-by-default, portability-by-manifest]
    assert downloader.protocol_pref == ProtocolPreference.SSH # constructed with no explicit args, config has prefer_ssh=True
    Suggested: Add fixture-based test writing prefer_ssh: true to temp config.json, constructing GitHubPackageDownloader() with no explicit args, asserting protocol_pref == ProtocolPreference.SSH.

  • [nit] No apm config unset prefer-ssh or allow-protocol-fallback CLI tests at tests/unit/test_config_command.py
    Proof (missing at unit): tests/unit/test_config_command.py::test_unset_prefer_ssh_removes_key -- proves: apm config unset prefer-ssh removes the key so subsequent get returns default (false)
    mock_update.assert_called_once_with({'prefer_ssh': None})
    Suggested: Add unset tests mirroring the copilot_cowork_skills_dir pattern.

  • [nit] Unit tests for precedence chain don't exercise GitHubPackageDownloader constructor wiring at tests/unit/test_protocol_config_precedence.py
    Proof (passed at unit): tests/unit/test_protocol_config_precedence.py::TestAllowProtocolFallbackPrecedence.test_env_var_empty_falls_through_to_config -- proves: Empty APM_ALLOW_PROTOCOL_FALLBACK env var falls through to config layer rather than being treated as falsy
    assert cfg_module.get_apm_allow_protocol_fallback() is True # config=True, env=''
    Suggested: No additional unit test needed here; covered by finding 1 (integration test).

This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • pypi.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "pypi.org"

See Network Configuration for more information.

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

  • #1308 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
  • feat: persist transport flags via apm config #1308 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #1308 · ● 5.8M ·

@github-actions github-actions Bot removed the panel-review Trigger the apm-review-panel gh-aw workflow label May 18, 2026
@Aaryan-Dadu
Copy link
Copy Markdown
Author

Are there any other changes that should be made in the PR @danielmeppiel?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support persisting CLI flags via apm config (e.g. allow-protocol-fallback, ssh)

4 participants